QUIC: Remove waiting on start event in send path #55442
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55302 Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events. Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code. Enable the assert about not throwing during event processing. Minor other cleanup.
|
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55302 Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events. Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code. Enable the assert about not throwing during event processing. Minor other cleanup.
|
|
Do we know for sure it is OK to send/receive before getting START_COMPLETED? |
| @@ -831,22 +818,10 @@ private static uint HandleEventPeerRecvAborted(State state, ref StreamEvent evt) | |||
| return MsQuicStatusCodes.Success; | |||
| } | |||
|
|
|||
| private static uint HandleEventStartComplete(State state) | |||
| private static uint HandleEventStartComplete(State state, ref StreamEvent evt) | |||
There was a problem hiding this comment.
Do we need to pass in the event if we are not using it? (now?)
There was a problem hiding this comment.
We don't need to. I added it because I thought I was going to use it, but since we don't have a definition for the START_COMPLETE event data, we can't right now. I don't think there's any harm in having it, though.
| // This is getting hit currently | ||
| // See https://github.com/dotnet/runtime/issues/55302 | ||
| //Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); | ||
| Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {evt.Type} event: {ex}"); |
There was a problem hiding this comment.
I think we should find better way how to propagate exceptions to the caller. This is going to be hard IMHO to debug in CI and does nothing to diagnose filed issues.
There was a problem hiding this comment.
I agree, it's a bit of a pain to deal with this.
That said, it's not obvious to me how to propagate this to the caller and/or make it easier to debug. Suggestions welcome.
Hopefully this is not a common occurrence -- an exception during a callback means something is deeply, deeply wrong.
That's how I read the msquic docs, yes. @nibanks Can you confirm? |
You can send at any time. All it does it queue if not started. |
BTW is there limit how much we would buffer? I'm wondering if we could create situation when caller think we are writing to the peer but we indeed simply create internal buffer. |
Yes, there's a limit to the send buffer. Roughly speaking, it works like winsock. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #55302
Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events.
Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code.
Enable the assert about not throwing during event processing.
Minor other cleanup.